[2 of 4] Builder operates on any HugrMut #281

acl-cqc · 2023-07-18T19:35:13Z

[note: this refers to the situation in commit 42d5eef. It has changed, hopefully for the better, since...]
Via a new Buildable trait, implemented for Hugr, &mut Hugr and &mut T for any T implementing HugrMut.

The end goal (not in this PR) is to allow Rewrites to work on any HugrMut, which in itself I am proposing as a way of allowing Rewrites to be invoked on a region of the Hugr (some future PR can impl HugrMut for FlatRegionView or similar.)

The changes here were mostly straightforward, but

It requires some nasty/grotesque syntax: <<<Self as Container>::Base as Buildable>::BaseMut<'_>> kind of thing. I'm wondering if this can be reduced by declaring trait Container: Buildable, say. (Of course, all this would go away if we'd accept a method returning dyn HugrMut....)
We have to make HugrMut public, which isn't ideal. But I guess we need to have some trait to indicate a Hugr or mutable view thereof - perhaps a sealed one?
The non-straightforward bit is that there are some long impl<T: HugrMut> HugrMut for &mut T blocks which are very obvious/tedious (3 of them - 1 for HugrMut and 2 for HugrView, so the latter 2 could be combined via a macro I guess)

…s broken

…sn't provide)

…asses

acl-cqc · 2023-07-25T12:51:35Z

So I think it should be possible to...

Make Buildable method return &mut BaseMut (rather than directly BaseMut).
- I think I tried this before and ran into the problem that it was impossible to get out the Hugr (if one has a Buildable<BaseMut=Hugr>), but maybe that's possible by a separate trait/method. This may be the biggest problem...
- Then we can drop impl <T:HugrMut> HugrMut for &mut T, hurrah
Remove BaseView, just return an immutable &BaseMut. (HugrMut: HugrView after all so that should suffice). This might allow removing some of the other BaseView impl blocks.
Probably, make Buildable a supertrait of Container (and any other similar flattenings that may be possible)

...and hopefully that'll make this palatable enough.

That's the plan, anyway...we'll see...

…ugr/&mut H -> H); return &mut BaseMut

…grMut

…rView)

acl-cqc · 2023-07-25T14:35:38Z

Make Buildable method return &mut BaseMut (rather than directly BaseMut).

Done. That seemed straightforward, didn't encounter any problem with retrieving the Hugr

Then we can drop impl <T:HugrMut> HugrMut for &mut T, hurrah

No. I did eventually drop the impl <T:HugrView+HugrInternals> HugrView for &T (keeping the same for &mut T), so that leaves only one nasty impl for each of HugrMut/HugrView.

Remove BaseView, just return an immutable &BaseMut. (HugrMut: HugrView after all so that should suffice).

That worked ok; BaseMut + BaseView => Base. Not sure what I meant by "This might allow removing some of the other BaseView impl blocks."...

Probably, make Buildable a supertrait of Container (and any other similar flattenings that may be possible)

This saved a massive amount of <<<Self as X>::Foo as Y>::Bar> stuff and makes everything reasonably readable. It probably means that we are passing around crazy references to references to references (all hidden behind those magic impl<T:HugrMut> HugrMut for &mut T), but it looks nice at least ;-)

So, please fire away, if you see a better way to do this then I'm game :-)

aborgna-q

Some code-specific comments. Let's discuss trait visibility irl

aborgna-q · 2023-07-25T15:12:48Z

src/hugr/view.rs

The impls for ref could use the delegate! macro (Same thing in hugrmut.rs). See

https://github.com/CQCL/portgraph/blob/5b181e871e975da25f7fc0c148b33b9097c827cf/src/view/filter.rs#L156-L173

aborgna-q · 2023-07-25T15:16:11Z

src/builder/build_traits.rs

@@ -1,3 +1,4 @@
+#![allow(missing_docs)]


Yes, this is not for submission, but good to flag up, sorry. If we decide to go this way then I'll take this out and put in docs, but at least it shows that the rest of CI passes

aborgna-q · 2023-07-25T15:21:12Z

src/builder/conditional.rs

+ pub fn case_builder(
+ &mut self,
+ case: usize,
+ ) -> Result<CaseBuilder<&mut <Self as Buildable>::Base>, BuildError> {


Isn't this

Suggested change

) -> Result<CaseBuilder<&mut <Self as Buildable>::Base>, BuildError> {

) -> Result<CaseBuilder<&mut B>, BuildError> {

?
(Same thing in other similar structs).

Oh, yeah, thank you! That definitely helps :-)

acl-cqc · 2023-07-25T17:13:25Z

Seyon wondered if we could make Buildable == HugrMut. However, then we need many types to implement HugrMut directly (that presently implement Buildable, i.e. they provide methods producing a HugrMut). I've tried a blanket impl<T: AsMut<HugrMut>> HugrMut for T but that doesn't work as you can't have AsMut<HugrMut> (only AsMut<dyn HugrMut> or AsMut<some concrete type implementing HugrMut>), and my attempt at impl<T: HugrMut, M: AsMut<HugrMut>> HugrMut for M failed with this rust error which does appear to say you can't do that. Any ideas @aborgna-q ?

acl-cqc · 2023-07-25T17:57:10Z

The problem with where we are now is that every time you create a new sub-builder (which internally calls HugrMut), we increase the level of indirection - originally you had a ModuleBuilder<Hugr>, say, then a FunctionBuilder<&mut Hugr>, then a CFGBuilder<&mut &mut Hugr> and inside that a BlockBuilder<&mut &mut &mut Hugr>. Every different <instantiation> is a fresh copy of the code in binary, and every level of &mut is probably another dereference on every call (to anything that uses the hugr_mut(), e.g. all the HugrMut methods)

So I think actually 42d5eef (returning Self::BaseMut) may have been the right thing, as I think that allows an XXXBuilder<&mut Hugr> to create a sub-builder that is also <&mut Hugr> and the number of levels of indirection does not increase....

…direction No blanket impl<F: Foo> Foo for &[mut] F (for Foo=HugrMut/HugrView/Buildable) Implement Buildable for <T: AsRef<Hugr> + AsMut<Hugr>> Buildable does not require HugrMut (too awkward as needs many impls) Revert hugrmut.rs and view.rs to branch main

acl-cqc · 2023-07-25T18:44:41Z

Ok, major changes, somewhat of a reversion. We are back to separate BaseMut and BaseView so that these types can include a reference only when necessary, thus we don't have to add a level of indirection each time we call hugr_mut().

In particular there are no impl<H:HugrMut> HugrMut for &mut H or similiar for HugrView/Buildable - there is the original impl<T:AsMut<Hugr> + AsRef<Hugr>> HugrMut for T as per branch main only (and the same for Buildable). Thus when you see a call to hugr_mut() you know it goes through at most one level of indirection, somewhat like 42d5eef and as opposed to the unlimited number of indirections possible in the previous version.

acl-cqc · 2023-07-25T19:25:46Z

src/builder/build_traits.rs

@@ -29,17 +30,40 @@ use crate::Hugr;

 use crate::hugr::HugrMut;

+pub trait Buildable {
+ type BaseMut<'a>: Buildable + HugrMut


We could avoid this Buildable + HugrMut everywhere if we wrote a blanket impl<B:Buildable> HugrMut for B (just by delegating to self.hugr_mut()). That would let us just constrain everything by Buildable instead, which would be nicer. OTOH it means one can execute HugrMut methods on every Buildable, of which there are many (every Container, etc.)....maybe that'd be a good thing (and could be done as a follow-up PR if we want to see what it looks like?)

acl-cqc · 2023-07-25T19:27:05Z

src/builder/build_traits.rs

+ fn hugr(&self) -> Self::BaseView<'_>;
+}
+
+impl<T: AsMut<Hugr> + AsRef<Hugr>> Buildable for T {


An interesting alternative (in that it illustrates what's going on) would be one impl Buildable for Hugr and a second impl<T: HugrMut + Copy> Buildable for T - as every &mut is Copy (right?!). That's two impl blocks rather than one though so I didn't...

acl-cqc · 2023-07-25T19:50:21Z

src/builder/cfg.rs

@@ -25,24 +25,28 @@ pub struct CFGBuilder<T> {
 pub(super) n_out_wires: usize,
 }

-impl<B: AsMut<Hugr> + AsRef<Hugr>> Container for CFGBuilder<B> {
+impl<B: Buildable> Buildable for CFGBuilder<B> {


There are, indeed, a lot of impl blocks like this. I guess these could be done via a macro...

Indeed, if/presuming that works, one could have a macro implementing HugrMut for all of these, which would allow combining Buildable into HugrMut. (If we want HugrMut-ness spread so widely).

acl-cqc · 2023-07-26T16:46:12Z

Closing in favour of #298

acl-cqc mentioned this pull request Jul 18, 2023

Rewrites work on any HugrMut #282

Merged

acl-cqc changed the title ~~Builder operates on any HugrMut~~ [2 of 4] Builder operates on any HugrMut Jul 18, 2023

Base automatically changed from refactor/rewrites_use_hugrmut_meths to main July 24, 2023 13:40

acl-cqc force-pushed the new/buildable_hugrmut branch from ec62652 to 19037c7 Compare July 24, 2023 13:46

acl-cqc added 7 commits July 24, 2023 14:55

[trivial][refactor]Combine identical impl blocks for BlockBuilder

4bc534c

[trivial][refactor]Combine identical impl blocks for TailLoopBuilder

5dcb6c7

Add Container::Base{Mut,View}, define as &'a (mut) Hugr - build_trait…

1dd1f1f

…s broken

WIP (changing region.rs - but needs impl of HugrView that view.rs doe…

51701cc

…sn't provide)

Expose HugrMut; add pub trait Buildable to parametrize all Builder cl…

65e02f1

…asses

build_traits.rs: allow missing docs

e8c0d81

clippy (inc 1 where VSCode was wrong)

5e64b0a

acl-cqc force-pushed the new/buildable_hugrmut branch from 19037c7 to 5e64b0a Compare July 24, 2023 14:02

Revert unnecessary test changes - from_existing(Hugr) still works

8ac5c6b

acl-cqc requested review from aborgna-q and ss2165 July 25, 2023 09:13

acl-cqc added 7 commits July 25, 2023 14:35

Merge remote-tracking branch 'origin/main' into new/buildable_hugrmut

42d5eef

deparameterize BaseMut but return &mut; combine impl Buildable for (H…

46c35a9

…ugr/&mut H -> H); return &mut BaseMut

Combine BaseMut/BaseView -> Base; remove explicit drop(!)

0372837

Buildable does not require HugrMut, but Base constrained Buildable+Hu…

630c216

…grMut

Container: Buildable. Massive savings on <gunk> albeit deeper refs?

6ae5516

Buildable::Base does not need to be Buildable!

1742bc9

Drop redundant +HugrView constraint, and HugrView impl for (&impl Hug…

2d6ebce

…rView)

aborgna-q reviewed Jul 25, 2023

View reviewed changes

acl-cqc added 3 commits July 25, 2023 18:24

<Self as Buildable>::Base -> B/T (whatever type param is)

cc8f489

Rewrite HugrMut impl using delegate

eee773c

Impl HugrView using delegate

f4cc4af

acl-cqc requested a review from aborgna-q July 25, 2023 18:45

acl-cqc commented Jul 25, 2023

View reviewed changes

aborgna-q mentioned this pull request Jul 26, 2023

Simplify trait structure in the project #284

Closed

4 tasks

acl-cqc closed this Jul 26, 2023

acl-cqc deleted the new/buildable_hugrmut branch August 3, 2023 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2 of 4] Builder operates on any HugrMut #281

[2 of 4] Builder operates on any HugrMut #281

acl-cqc commented Jul 18, 2023 •

edited

Loading

acl-cqc commented Jul 25, 2023

acl-cqc commented Jul 25, 2023

aborgna-q left a comment

aborgna-q Jul 25, 2023

aborgna-q Jul 25, 2023

acl-cqc Jul 25, 2023

aborgna-q Jul 25, 2023

acl-cqc Jul 25, 2023

acl-cqc commented Jul 25, 2023

acl-cqc commented Jul 25, 2023

acl-cqc commented Jul 25, 2023

acl-cqc Jul 25, 2023

acl-cqc Jul 25, 2023

acl-cqc Jul 25, 2023

acl-cqc Jul 25, 2023

acl-cqc commented Jul 26, 2023

	) -> Result<CaseBuilder<&mut <Self as Buildable>::Base>, BuildError> {
	) -> Result<CaseBuilder<&mut B>, BuildError> {

[2 of 4] Builder operates on any HugrMut #281

[2 of 4] Builder operates on any HugrMut #281

Conversation

acl-cqc commented Jul 18, 2023 • edited Loading

acl-cqc commented Jul 25, 2023

acl-cqc commented Jul 25, 2023

aborgna-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jul 25, 2023

acl-cqc commented Jul 25, 2023

acl-cqc commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jul 26, 2023

acl-cqc commented Jul 18, 2023 •

edited

Loading